Add capability to raise JwtBearer events#86
Conversation
|
Warning Rate limit exceeded@Shane32 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes introduce configurable JWT Bearer authentication support for GraphQL WebSocket connections. A new options class and extension methods allow enabling or customizing JWT event handling. The WebSocket authentication service is updated to trigger JWT authentication events based on configuration. Associated tests and API approval files are updated to reflect the new features and public API surface. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLServer
participant JwtWebSocketAuthenticationService
participant JwtBearerEvents
Client->>GraphQLServer: Initiate WebSocket connection with JWT
GraphQLServer->>JwtWebSocketAuthenticationService: AuthenticateAsync(request)
alt EnableJwtEvents = true
JwtWebSocketAuthenticationService->>JwtBearerEvents: Trigger MessageReceived
alt MessageReceived handled
JwtWebSocketAuthenticationService-->>GraphQLServer: Return principal (early exit)
else
JwtWebSocketAuthenticationService->>JwtBearerEvents: Trigger TokenValidated
alt TokenValidated failed
JwtWebSocketAuthenticationService-->>GraphQLServer: Continue to next scheme
else
JwtWebSocketAuthenticationService-->>GraphQLServer: Return principal
end
end
alt Token validation exception
JwtWebSocketAuthenticationService->>JwtBearerEvents: Trigger AuthenticationFailed
alt AuthenticationFailed handled
JwtWebSocketAuthenticationService-->>GraphQLServer: Return principal (early exit)
end
end
else EnableJwtEvents = false
JwtWebSocketAuthenticationService-->>GraphQLServer: Standard JWT validation
end
GraphQLServer-->>Client: WebSocket authenticated or rejected
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
src/GraphQL.AspNetCore3.JwtBearer/JwtWebSocketAuthenticationService.cs (2)
78-110:⚠️ Potential issuePossible
nulltoken afterMessageReceivedeventIf the
OnMessageReceivedcallback setscontext.Token = nullto suppress
authentication,tokenbecomesnull, yet the subsequent validators call
validator.CanReadToken(token)which will throwArgumentNullException.Consider short-circuiting when the event nullifies the token:
token = messageResult.Token; +if (string.IsNullOrEmpty(token)) + continue; // event intentionally suppressed authentication for this scheme🧰 Tools
🪛 GitHub Actions: Check formatting
[error] 94-94: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\s'.
[error] 97-97: WHITESPACE: Fix whitespace formatting. Replace 25 characters with '\s'.
[error] 99-99: WHITESPACE: Fix whitespace formatting. Replace 29 characters with '\s'.
[error] 106-106: WHITESPACE: Fix whitespace formatting. Replace 49 characters with '\n '.
1-333:⚠️ Potential issueFormatting violations break the CI pipeline
dotnet format(or your repo’s formatting tool) reports >30 whitespace errors
(lines 94, 97, 99 … 310). Please run the formatter to unblock CI.🧰 Tools
🪛 GitHub Actions: Check formatting
[error] 94-94: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\s'.
[error] 97-97: WHITESPACE: Fix whitespace formatting. Replace 25 characters with '\s'.
[error] 99-99: WHITESPACE: Fix whitespace formatting. Replace 29 characters with '\s'.
[error] 106-106: WHITESPACE: Fix whitespace formatting. Replace 49 characters with '\n '.
[error] 161-161: WHITESPACE: Fix whitespace formatting. Replace 73 characters with '\n '.
[error] 162-162: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\n '.
[error] 163-163: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\s'.
[error] 166-166: WHITESPACE: Fix whitespace formatting. Replace 41 characters with '\s'.
[error] 170-170: WHITESPACE: Fix whitespace formatting. Replace 81 characters with '\n '.
[error] 173-173: WHITESPACE: Fix whitespace formatting. Replace 73 characters with '\n '.
[error] 174-174: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\n '.
[error] 179-179: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\s'.
[error] 182-182: WHITESPACE: Fix whitespace formatting. Replace 41 characters with '\s'.
[error] 188-188: WHITESPACE: Fix whitespace formatting. Replace 73 characters with '\n '.
[error] 189-189: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\n '.
[error] 190-190: WHITESPACE: Fix whitespace formatting. Replace 33 characters with '\n '.
[error] 232-232: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\n '.
[error] 233-233: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 238-238: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 242-242: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 244-244: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 245-245: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n '.
[error] 246-246: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 260-260: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\n '.
[error] 261-261: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 267-267: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 271-271: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 273-273: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 274-274: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n '.
[error] 275-275: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 279-279: WHITESPACE: Fix whitespace formatting. Replace 13 characters with '\s'.
[error] 292-292: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\n '.
[error] 293-293: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 298-298: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 302-302: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 304-304: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 305-305: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n '.
[error] 306-306: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 310-310: WHITESPACE: Fix whitespace formatting. Replace 13 characters with '\s'.
🧹 Nitpick comments (3)
src/GraphQL.AspNetCore3.JwtBearer/AspNetCore3JwtBearerExtensions.cs (1)
18-21: Nit: avoid the empty lambda allocationThe parameterless overload allocates a new delegate (
options => { }) on every
call. A cached static delegate removes that tiny, but unnecessary, allocation.-private static readonly Action<JwtBearerAuthenticationOptions> _noOp = _ => { }; -… -=> builder.AddJwtBearerAuthentication(_noOp); +private static readonly Action<JwtBearerAuthenticationOptions> _noOp = _ => { }; +… +=> builder.AddJwtBearerAuthentication(_noOp);src/GraphQL.AspNetCore3.JwtBearer/JwtWebSocketAuthenticationService.cs (2)
120-131: Duplicate event-handling logic
TokenValidatedlogic is repeated for both NET8 and legacy branches. Extract
the common fragment into a local function to improve readability and reduce the
risk of divergent behaviour in the future.
319-325:EventResult.Tokenshould be nullable
Tokenis empty-string initialised, but callers interpret empty as a valid
value and overwritetokenwith it. Make it nullable (string?) to
differentiate not set from intentionally cleared.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/GraphQL.AspNetCore3.JwtBearer/AspNetCore3JwtBearerExtensions.cs(1 hunks)src/GraphQL.AspNetCore3.JwtBearer/JwtBearerAuthenticationOptions.cs(1 hunks)src/GraphQL.AspNetCore3.JwtBearer/JwtWebSocketAuthenticationService.cs(6 hunks)src/Tests.ApiApprovals/ApiApprovalTests.cs(2 hunks)src/Tests.ApiApprovals/GraphQL.AspNetCore3.JwtBearer.approved.txt(1 hunks)src/Tests.ApiApprovals/Tests.ApiTests.csproj(1 hunks)src/Tests/JwtBearer/AspNetCore3JwtBearerExtensionsTests.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Tests/JwtBearer/AspNetCore3JwtBearerExtensionsTests.cs (1)
src/GraphQL.AspNetCore3.JwtBearer/JwtBearerAuthenticationOptions.cs (1)
JwtBearerAuthenticationOptions(8-17)
src/GraphQL.AspNetCore3.JwtBearer/AspNetCore3JwtBearerExtensions.cs (1)
src/GraphQL.AspNetCore3.JwtBearer/JwtBearerAuthenticationOptions.cs (1)
JwtBearerAuthenticationOptions(8-17)
🪛 GitHub Actions: Check formatting
src/Tests/JwtBearer/AspNetCore3JwtBearerExtensionsTests.cs
[error] 28-28: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\n '.
[error] 29-29: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n '.
src/GraphQL.AspNetCore3.JwtBearer/JwtWebSocketAuthenticationService.cs
[error] 94-94: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\s'.
[error] 97-97: WHITESPACE: Fix whitespace formatting. Replace 25 characters with '\s'.
[error] 99-99: WHITESPACE: Fix whitespace formatting. Replace 29 characters with '\s'.
[error] 106-106: WHITESPACE: Fix whitespace formatting. Replace 49 characters with '\n '.
[error] 161-161: WHITESPACE: Fix whitespace formatting. Replace 73 characters with '\n '.
[error] 162-162: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\n '.
[error] 163-163: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\s'.
[error] 166-166: WHITESPACE: Fix whitespace formatting. Replace 41 characters with '\s'.
[error] 170-170: WHITESPACE: Fix whitespace formatting. Replace 81 characters with '\n '.
[error] 173-173: WHITESPACE: Fix whitespace formatting. Replace 73 characters with '\n '.
[error] 174-174: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\n '.
[error] 179-179: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\s'.
[error] 182-182: WHITESPACE: Fix whitespace formatting. Replace 41 characters with '\s'.
[error] 188-188: WHITESPACE: Fix whitespace formatting. Replace 73 characters with '\n '.
[error] 189-189: WHITESPACE: Fix whitespace formatting. Replace 37 characters with '\n '.
[error] 190-190: WHITESPACE: Fix whitespace formatting. Replace 33 characters with '\n '.
[error] 232-232: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\n '.
[error] 233-233: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 238-238: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 242-242: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 244-244: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 245-245: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n '.
[error] 246-246: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 260-260: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\n '.
[error] 261-261: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 267-267: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 271-271: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 273-273: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 274-274: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n '.
[error] 275-275: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 279-279: WHITESPACE: Fix whitespace formatting. Replace 13 characters with '\s'.
[error] 292-292: WHITESPACE: Fix whitespace formatting. Replace 21 characters with '\n '.
[error] 293-293: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 298-298: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 302-302: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 304-304: WHITESPACE: Fix whitespace formatting. Replace 17 characters with '\n '.
[error] 305-305: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n '.
[error] 306-306: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\s'.
[error] 310-310: WHITESPACE: Fix whitespace formatting. Replace 13 characters with '\s'.
🪛 LanguageTool
src/Tests.ApiApprovals/GraphQL.AspNetCore3.JwtBearer.approved.txt
[duplication] ~11-~11: Possible typo: you repeated a word.
Context: ...ateAsync(GraphQL.AspNetCore3.WebSockets.AuthenticationRequest authenticationRequest) { } public sealed class AuthPa...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (6)
src/Tests.ApiApprovals/Tests.ApiTests.csproj (1)
19-19: Project reference correctly addedThe added reference to the JwtBearer project is necessary for API approval tests to properly validate the public API surface of the new JWT functionality.
src/Tests.ApiApprovals/ApiApprovalTests.cs (2)
2-2: Namespace import correctly addedAdding the JwtBearer namespace import is necessary to reference the new service type in the tests.
16-16: API approval test coverage properly extendedIncluding JwtWebSocketAuthenticationService in the API approval tests ensures that its public API surface is verified and tracked for any future changes.
src/GraphQL.AspNetCore3.JwtBearer/JwtBearerAuthenticationOptions.cs (1)
1-17: Well-designed options class for JWT Bearer authenticationThe new JwtBearerAuthenticationOptions class is well-structured and follows the standard .NET options pattern. The EnableJwtEvents property is clearly named and well-documented, making it easy for users to understand how to configure JWT event handling for WebSocket connections.
src/GraphQL.AspNetCore3.JwtBearer/JwtWebSocketAuthenticationService.cs (1)
137-146:AuthenticationFailedevent: make success path explicitIf the
AuthenticationFailedevent setscontext.NoResult()the current code
treats it as not handled (Handled = true, Success = false) and silently
continues to the next scheme – good. However, when the event sets
context.Success()but does not assigncontext.Principal, a subsequent
NRE will be thrown when you dereferencefailedResult.Principal!. Defensive
check recommended.src/Tests.ApiApprovals/GraphQL.AspNetCore3.JwtBearer.approved.txt (1)
1-27: No review needed – generated approval fileThis file is auto-generated by the public-API approval tests. Changes simply
reflect the new public surface and do not need manual review.🧰 Tools
🪛 LanguageTool
[duplication] ~11-~11: Possible typo: you repeated a word.
Context: ...ateAsync(GraphQL.AspNetCore3.WebSockets.AuthenticationRequest authenticationRequest) { } public sealed class AuthPa...(ENGLISH_WORD_REPEAT_RULE)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(0 hunks)src/Tests/JwtBearer/JwtWebSocketAuthenticationServiceTests.cs(1 hunks)src/Tests/Tests.csproj(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/test.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (2)
src/Tests/JwtBearer/JwtWebSocketAuthenticationServiceTests.cs (1)
256-256: LGTM: This correctly enables JWT event handling for testsThe addition of the
trueparameter toAddJwtBearerAuthentication()properly enables the new JWT event handling capability for these tests, aligning with the PR objective of adding JWT Bearer event trigger support.src/Tests/Tests.csproj (1)
7-7: Update Windows test configurations to match supported TFMs
The Windows-specific<TargetFrameworks>group now includesnet48;netcoreapp2.1;net6.0;net8.0. Ensure that all conditional<PackageReference>entries (e.g., JwtBearer, Mvc.Testing) correctly cover these frameworks and that version constraints remain accurate.
Coverage Report
|
Shane32
changed the title
Pull Request Test Coverage Report for Build 14892854898Details
💛 - Coveralls |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests